Fix Android MediaPicker result recovery#35455
Conversation
|
Hey there @@AdamEssenmacher! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35455Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35455" |
github-actions
Bot
added
the
area-essentials
PureWeen
added
area-essentials-mediapicker
platform/android
t/bug
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/review -b feature/regression-check -p android |
MauiBot
added
s/agent-review-incomplete
s/agent-reviewed
|
/review -b feature/regression-check -p android |
1 similar comment
|
/review -b feature/regression-check -p android |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 10 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
🤖 Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against #1 automatically generated candidates and selected try-fix-1 as the strongest fix.
Why: try-fix-1 is the smallest correct fix: it ships the PR's Java pruning (the actual .NET 8->10.0.60 regression root cause) plus the AndroidX ActivityResultLauncher switch, while deleting the 3265-line speculative cross-process recovery layer (state machine, hand-rolled v1-v5 serializer, 3-method public API, 50+ device tests). It resolves all 10 reviewer findings (most as N/A), eliminates the gate's CS0260/CS0115 build errors and the failing MediaPickerRecovery_Tests, and ships zero new public API. The PR (and pr-plus-reviewer) failed the gate; try-fix candidates were predicted-pass (no Helix infra in this session, disclosed in the report).
Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.
Candidate diff (`try-fix-1`)
diff --git a/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java b/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java
index 3b2b9d5551..72dd2e65fa 100644
--- a/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java
+++ b/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformMauiAppCompatActivity.java
@@ -1,7 +1,5 @@
package com.microsoft.maui;
-import android.content.Context;
-import android.content.res.Resources;
import android.content.res.TypedArray;
import androidx.appcompat.app.AppCompatActivity;
@@ -12,11 +10,28 @@ import android.os.Bundle;
* Class for batching native method calls within the MauiAppCompatActivity implementation
*/
public class PlatformMauiAppCompatActivity {
+ // These are Android framework / AndroidX saved-instance-state keys. MAUI does not create
+ // the bundles stored under these keys; it only removes or preserves them before AppCompat
+ // restores saved state. AndroidX does not expose public constants for these values.
+ //
+ // ComponentActivity saves pending ActivityResultRegistry state here. Preserving this bundle
+ // lets AndroidX replay pending activity results after activity or process recreation.
+ private static final String ACTIVITY_RESULT_REGISTRY_KEY = "android:support:activity-result";
+
+ // Framework FragmentManager and AndroidX FragmentManager saved-state keys. MAUI removes these
+ // when fragment restore is disabled because restoring old platform fragments can conflict with
+ // MAUI's own navigation/window reconstruction.
+ private static final String ANDROID_FRAGMENTS_KEY = "android:fragments";
+ private static final String SUPPORT_FRAGMENTS_KEY = "android:support:fragments";
+
+ // SavedStateRegistry's top-level bundle key. Older MAUI behavior removed this whole bundle to
+ // suppress fragment restore side effects, but that also discarded ActivityResultRegistry state.
+ private static final String SAVED_STATE_REGISTRY_KEY = "androidx.lifecycle.BundlableSavedStateRegistry.key";
+
public static void onCreate(AppCompatActivity activity, Bundle savedInstanceState, boolean allowFragmentRestore, int splashAttr, int mauiTheme)
{
if (!allowFragmentRestore && savedInstanceState != null) {
- savedInstanceState.remove("android:support:fragments");
- savedInstanceState.remove("androidx.lifecycle.BundlableSavedStateRegistry.key");
+ removeFragmentRestoreState(savedInstanceState);
}
boolean mauiSplashAttrValue = false;
@@ -33,4 +48,30 @@ public class PlatformMauiAppCompatActivity {
activity.setTheme(mauiTheme);
}
}
+
+ private static void removeFragmentRestoreState(Bundle savedInstanceState)
+ {
+ // First remove the direct fragment entries that may be present in the activity state.
+ savedInstanceState.remove(ANDROID_FRAGMENTS_KEY);
+ savedInstanceState.remove(SUPPORT_FRAGMENTS_KEY);
+
+ Bundle savedStateRegistry = savedInstanceState.getBundle(SAVED_STATE_REGISTRY_KEY);
+ if (savedStateRegistry != null) {
+ // The saved-state registry is a shared AndroidX container. Extract the activity-result
+ // entry before removing the container so pending activity results are not lost with the
+ // fragment-related providers.
+ Bundle activityResultRegistryState = savedStateRegistry.getBundle(ACTIVITY_RESULT_REGISTRY_KEY);
+
+ savedInstanceState.remove(SAVED_STATE_REGISTRY_KEY);
+
+ if (activityResultRegistryState != null) {
+ // Keep only the AndroidX ActivityResultRegistry state needed to replay pending
+ // results after activity/process recreation. Other saved-state providers may
+ // contain fragment state that MAUI cannot safely restore.
+ Bundle prunedSavedStateRegistry = new Bundle();
+ prunedSavedStateRegistry.putBundle(ACTIVITY_RESULT_REGISTRY_KEY, activityResultRegistryState);
+ savedInstanceState.putBundle(SAVED_STATE_REGISTRY_KEY, prunedSavedStateRegistry);
+ }
+ }
+ }
}
diff --git a/CustomAgentLogsTmp/PRState/35455/PRAgent/try-fix-1/csharp-sketch.md b/CustomAgentLogsTmp/PRState/35455/PRAgent/try-fix-1/csharp-sketch.md
new file mode 100644
index 0000000..1111111
--- /dev/null
+++ b/CustomAgentLogsTmp/PRState/35455/PRAgent/try-fix-1/csharp-sketch.md
@@ -0,0 +1,30 @@
+# try-fix-1 C# sketch (companion to Java diff)
+
+The Java hunk above IS the regression fix. The C# delta required to use AndroidX
+launchers (so AndroidX has a registered launcher to replay results into) is:
+
+1. Add three small files (no recovery coupling):
+ * `src/Essentials/src/Platform/CapturePhotoForResult.android.cs` -- thin
+ `ActivityForResultRequest<TakePicture, JavaBoolean>` singleton, registered
+ in `ActivityStateManager` at activity-create time.
+ * `src/Essentials/src/Platform/CaptureVideoForResult.android.cs` -- same
+ pattern for `CaptureVideo` contract.
+ * `src/Essentials/src/Platform/PickVisualMediaForResult.android.cs` /
+ `PickMultipleVisualMediaForResult.android.cs` -- thin AndroidX launcher
+ wrappers that materialize URIs **on Task.Run** (fixes finding F1/F2).
+
+2. Modify `src/Essentials/src/MediaPicker/MediaPicker.android.cs` to use
+ `CapturePhotoForResult.Instance.Launch(outputUri)` /
+ `CaptureVideoForResult.Instance.Launch(outputUri)` instead of the legacy
+ `IntermediateActivity.StartAsync(captureIntent, requestCode, ...)`. The
+ AndroidX launcher MUST be registered before STARTED, which is achieved by
+ adding a one-line registration in `ActivityStateManager.OnActivityCreated`.
+
+3. NO new public API. NO `MediaPickerRecovery.android.cs`. NO
+ `MediaPickerRecoveryManager.android.cs`. NO `MediaPickerRecoveryStore`.
+ NO new entries in `PublicAPI.Unshipped.txt`. NO `MediaPickerRecovery_Tests`.
+
+4. On orphaned-result (process death + AndroidX replay): the registered
+ launcher's no-op `OnActivityResultForOrphanedLaunch` swallows the boolean
+ result; the captured photo file is left on disk in the app's cache and the
+ user re-taps the camera button. Accepted UX trade-off.
|
/review -b feature/regression-check -p android |
AI code review for net11.0 targetVerdict: Needs discussion Automated, non-approval review comment (no human approval implied). Reviewed the Android MediaPicker changes and the new recovery subsystem independently before the PR narrative. This is a large change (~4.1k LOC incl. tests) adding a new public API surface, so a careful human pass is warranted. Observations (no blocking bug found)
Confidence / CIConfidence: medium (breadth + new concurrency/recovery semantics; relied on diff reading, not on-device runs). CI: build/unit legs pass, Build Analysis pending; no PR-attributable failures observed. |
|
Agree with the 'Needs discussion' verdict. That's what I started with on my original comment on the underlying issue Some high-level discussion points I think are important to frame any follow-up:
|
AI code review refresh for net11.0 targetVerdict: Needs discussion (unchanged from round 1 — code unchanged, awaiting maintainer/API-review) Automated, non-approval review comment (no human approval implied). Refresh triggered by @AdamEssenmacher's framing reply. Re-read the diff and re-inspected Head reviewed: Reconciliation with author's discussion pointsI agree with the framing in the latest comment — these are maintainer-level decisions, not code defects:
Code observations from this pass
CI note
Round-1 findings statusAll five round-1 observations remain the only items I'd raise; nothing new bug-level surfaced on this re-read. Three of them (API surface needs API-review, file-lifetime safety, Confidence
Explicit non-approval disclaimerThis comment is not an approval, does not request changes via the GitHub review system, and is posted only as an inspection/discussion artifact. A human maintainer with API-review authority must make the final call on the new |
|
@AdamEssenmacher great work! The recovery event is a super API addition! I'm an outsider and have solved this exact problem earlier. I want to share some insights which required quite a bit of debugging to figure out. Perhaps others can save time if this is being documented in MAUI. WebViewWhat compounds this problem and makes it far worse is any in-app usage of The not so known pain of using WebViewWhen WebView shuts down it makes sure to shut down any app using the WebView-component for the entire phone. This is by design. It normally happens when the WebView-component (strictly a separate app running as its own process) is being updated. In essence the shutdown of WebView leads to a cascading shutdown of apps. Any usage at any point in time of WebView will make the app eligible for shutdownLet's say the MAUI app triggers an OAuth login that happens to use WebView as part of the login process. This is enough. Even though the WebView is shown only once, the tie between the MAUI app and the WebView is forever. Perhaps this is the nature of the Android WebView-component itself. Perhaps it's something that could be improved in the MAUI code. Perhaps that was the fact on the one phone I used for debugging. I'll leave it for the reader to figure it out.
@kubaflo Hope this can make it into the MAUI documentation. |
|
This is output from an Android log session when force stopping the WebView. I removed lines not related to the issue, but the order is kept. Look for Output |
AI code review refresh for net11.0 targetVerdict: Needs discussion (unchanged from rounds 1/3 — no code change; new activity is community discussion, not a defect) Automated, non-approval review comment (no human approval implied). Refresh triggered by @espenrl's two new comments after the round-3 marker. Re-checked the head, CI, and re-read the changed Android MediaPicker / recovery code before responding. Head reviewed: What changed since round 3
Assessment of the new discussion
Code re-check (unchanged head)
CI note
Confidence
Explicit non-approval disclaimerThis comment is not an approval and does not request changes via the GitHub review system. It is an inspection/discussion artifact only. A human maintainer with API-review authority owns the final decision on the new recovery API surface and on any documentation follow-ups (including @espenrl's WebView/ |
| /// <param name="id">The identifier of the recovered result.</param> | ||
| /// <param name="kind">The kind of MediaPicker operation that produced the result.</param> | ||
| /// <param name="files">The recovered media files.</param> | ||
| public RecoveredMediaPickerResult(string id, RecoveredMediaPickerResultKind kind, IReadOnlyList<FileResult> files) |
There was a problem hiding this comment.
@AdamEssenmacher This otherwise looks good to me. One API-surface question: does RecoveredMediaPickerResult need a public constructor? Since these results are produced by MAUI recovery APIs rather than by app code, making the constructor internal would reduce the permanent public surface while still letting consumers read Id, Kind, and Files. If there’s a consumer construction scenario here, it would be good to call that out; otherwise I’d lean toward making this internal.
AI code review refresh for net11.0 targetVerdict: Needs discussion (carry-over from rounds 1/3/5; a new maintainer API-surface question is now the only open item) Automated, non-approval review comment (no human approval implied). Refresh triggered by @jfversluis's inline review at 2026-06-08T13:21:32Z on Head reviewed: a38f919 (committed 2026-05-24T22:13:26Z — no new commits since round 5) What changed since round 5
Independent assessment of the API-surface pointI agree with @jfversluis, and I think the case for making the ctor
This is a one-line visibility change plus a one-line Devil's advocate
Status of prior findingsAll ten MauiBot expert-review findings (rounds 1–2) and the rubber-duck pass remain addressed in code at the current head; nothing has regressed. CI note
No AzDO Confidence
This comment is informational only. No approval implied — that decision is for the maintainers. |
AI code review refresh for net11.0 targetVerdict: LGTM — the round-12 “Needs discussion” item is resolved. No prior findings remain open. Automated, non-approval review comment (no human approval implied). Refresh triggered by a new commit at Head reviewed: What changed since round 12Single, surgical commit
That is exactly the API-surface narrowing called for in round 12 (and previously by @jfversluis's inline review), and it does not touch any of the recovery, lifecycle, permission, exception-handling, or concurrency code that prior rounds 1–11 scrutinized. Independent verification of the change
Devil's advocate
Status of prior findingsAll MauiBot expert-review items, rubber-duck-pass items, and the round-12 API-surface question are addressed at head. No open carry-over. CI note
No AzDO Confidence
This comment is informational only. No approval implied — that decision is for the maintainers. |
6 participants
Description of Change
Android can destroy or recreate an app process while another activity is in front. This is especially relevant for camera flows: the Android Activity Result documentation explicitly calls out memory-intensive operations such as camera usage as cases where the launching process/activity may be destroyed, and says result callbacks must be registered unconditionally when the activity is recreated:
https://developer.android.com/training/basics/intents/result
When this happens today, MAUI’s original
MediaPickertask is gone. The user can successfully finish the system camera or picker UI, but the app has no reliable way to receive the result. On affected device/app configurations, this effectively makes photo and/or video capture unusable throughMediaPicker.This change adds Android-only recovery support for AndroidX-backed
MediaPickeractivity results. MAUI now registers the relevant AndroidX activity-result launchers early, persists the active MediaPicker operation before launch, durably records accepted AndroidX callback results, and exposes an opt-in recovery surface so apps can retrieve results after process/activity recreation.The recovery surface is additive and Android-only:
MediaPicker.GetRecoveredMediaPickerResultsAsync()MediaPicker.WaitForRecoveredMediaPickerResultsAsync(CancellationToken)MediaPicker.ClearRecoveredMediaPickerResultAsync(string id)Recovery covers AndroidX-backed MediaPicker flows across the board:
Picker URI handling follows Android’s photo picker guidance around persisted media access:
https://developer.android.com/training/data-storage/shared/photo-picker#persist-media-file-access
Normal live-process behavior is unchanged: existing
MediaPickerandIMediaPickermethods still complete normally when the app survives, and no duplicate recovered result is queued.This also adds Android device-test coverage for the recovery state machine, callback routing, cancellation/wait behavior, duplicate prevention, picker URI materialization, and capture photo processing safety. I also smoke-tested the happy path manually on an API 36 AVD for photo capture, video capture, photo pick, video pick, and multi-photo pick.
Issues Fixed
Fixes #35308